-
-
Notifications
You must be signed in to change notification settings - Fork 384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(transformer/react): correct import binding names #3014
feat(transformer/react): correct import binding names #3014
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
@@ -79,6 +82,7 @@ impl<'a> Transformer<'a> { | |||
|
|||
impl<'a> VisitMut<'a> for Transformer<'a> { | |||
fn visit_program(&mut self, program: &mut Program<'a>) { | |||
self.ctx.borrow_mut().naming.visit_program(program); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to collect all bindings first.
CodSpeed Performance ReportMerging #3014 will degrade performances by 4.13%Comparing Summary
Benchmarks breakdown
|
|
||
pub type Ctx<'a> = Rc<TransformCtx<'a>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransformerNaming
needs to mutation.
In a previous implementation. All the TransformerCtx
fields that cannot be copied are wrapped in Rc
oxc/crates/oxc_transformer/src/context.rs
Lines 15 to 22 in 31ed532
#[derive(Clone)] | |
pub struct TransformerCtx<'a> { | |
pub ast: Rc<AstBuilder<'a>>, | |
pub options: Cow<'a, TransformOptions>, | |
semantic: Rc<RefCell<Semantic<'a>>>, | |
errors: Rc<RefCell<Vec<Error>>>, | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big change.
It probably makes more sense to use interior mutability in TransformerNaming
, instead of adding all these borrow calls everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes more sense to use interior mutability in
TransformerNaming
, instead of adding all these borrow calls everywhere.
Yes, I agree. When we finally consider this PR, I'll refactor it.
Let me iterate on this tomorrow. |
I'll find sometime to figure out recreating scopes and bindings during visits, probably this weekend. |
The problem we had before is that the nodes don't have IDs, so there was no easy way to back reference. That's why @rzvxa was trying to add IDs to everything. |
We still can add this, I didn't receive any pushback while implementing it in #2876 and #2818, I just stopped to figure out the traverse first; Now that the traverse is almost sorted out we may want to revisit the ast_node_id. |
I suspect that by adding node_ids to the nodes we can do a minimal refactor for linters which removes a bunch of useless checks and improves the performance by like 10% to 20%. We can still provide a way to handle AstNodes so we don't have to refactor them all from day one, But we can offer additional traits to intercept the inner node without needing extra match on AstKind. Edit:The whole reason behind AstNode is having semantic information along with our nodes. |
I'm just thinking this one out loud; Since now we have a parent pointer - even though it is opaque - we might be able to map between that and our own ast_node_id so we don't have to add an extra For example if we always allocate all of our nodes in the same buffer and we have the root of our buffer we can find our relative position to that and use it to fetch our relative data in a different buffer similar to an indexing operation. |
Finally, we have a |
No description provided.